Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IOS-8050: Apply rounding to the Hedera fee value #850

Merged
merged 1 commit into from
Sep 23, 2024

Conversation

m3g0byt3
Copy link
Contributor

@m3g0byt3 m3g0byt3 commented Sep 23, 2024

IOS-8050

В общем корневая причина проблемы - это то что у хедеры фии номинируются в USD.
И соответственно есть exchange rate usd/hbar

Использование этого exchange rate приводит к потери точности - поэтому добавил округление up

@m3g0byt3 m3g0byt3 added the release ASAP to review label Sep 23, 2024
Comment on lines 424 to +428
let feeValue = exchangeRate.nextHBARPerUSD * feeBase * Constants.maxFeeMultiplier
let feeAmount = Amount(with: walletManager.wallet.blockchain, value: feeValue)
// Hedera fee calculation involves conversion from USD to HBar units, which ultimately results in a loss of precision.
// Therefore, the fee value is always approximate and rounding of the fee value is mandatory.
let feeRoundedValue = feeValue.rounded(blockchain: walletManager.wallet.blockchain, roundingMode: .up)
let feeAmount = Amount(with: walletManager.wallet.blockchain, value: feeRoundedValue)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Вот тут я вообще не понял:

я изначально добавил .rounded(blockchain: walletManager.wallet.blockchain, roundingMode: .up) только для exchangeRate.nextHBARPerUSD - так как именно этот компонент привносил избыточную точность, он был чем-то вроде 17.9466625137819201082727172

Я округляю его до scale равного Hedera decimal count (это 8) - и получаю 17.94666251

Затем домножаю в 424 строке эту величину на feeBase (0.05 в случае простого трансфера) и maxFeeMultiplier (1.1) и ожидаю, что scale результата будет меньше или равен наибольшему scale множителей (то есть 8)

В итоге получаю 0.98706643805 со scale равным 11, что естественно приводит к дробному числу во fromAmount ¯_(ツ)_/¯

Поэтому пришлось применить округление на финальное значение

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Логично, это же просто число, округленное, после последующих операций не гарантируется сохранение оригинального числа знаков после запятой

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Так может тогда имеет смысл оставить округление только на последнем этапе, чтобы не терять точность лишний раз?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

На последнем это каком? Непосредственно в экспрессе?

Вообще здесь и есть последний этап внутри воллет менеджера - перед отдачей посчитанного fee наружу в estimateFee (что и вызывало ошибку в экспрессе)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Да я это и имел в виду. Ты пишешь, округлил, потом помножил, потом надо опять округлять. Вот и предложил не округлять на промежуточных этапах, но раз уже так, то ок

Copy link
Contributor Author

@m3g0byt3 m3g0byt3 Sep 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Эта логика специфична для Хедеры и может аффектить обычные транзы, а не только экспресс - поэтому округление точно нужно тут

Но в экспрессе по хорошему тоже сделать бы округление перед отправкой запроса (со scale == decimal count), раз у бэка такой строгий контракт - чтобы ситуация как с хедерой не повторилась с другими сетями

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ты пишешь, округлил, потом помножил, потом надо опять округлять.

ну и это я же писал что пробовал и что не взлетело)
сейчас двойного округления нет

@m3g0byt3 m3g0byt3 marked this pull request as ready for review September 23, 2024 13:50
@m3g0byt3 m3g0byt3 requested review from tureck1y, dbaturin and a team as code owners September 23, 2024 13:50
@tureck1y tureck1y merged commit 4f3f20e into develop Sep 23, 2024
1 check passed
@tureck1y tureck1y deleted the bugfix/IOS-8050_hedera_swap_error branch September 23, 2024 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release ASAP to review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants